Skip to content

Conversation

@jgallimore
Copy link
Contributor

No description provided.

catch (Exception e)
{
logger.info("Error occurred: " + e.getMessage());
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the e.printStackTrace is a left over?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, yes, I forgot to remove that in my excitement to open the PR. :-)

@jgallimore
Copy link
Contributor Author

Hi @struberg , thanks for the review and feedback. Does the overall approach look ok, or is there a better way to achieve this?


logger.fine("Deploying configurations from XML has ended successfully.");

try
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder why the url is not in the scanner since it should so we do not need that hack at all in impl

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping there would be a public static final URL CDI_STANDALONE = ... somewhere, in order to avoid this. org.apache.openwebbeans.se.CDISeBeanArchiveService#EMBEDDED_URL was the closest I could find, but is a String as opposed to a URL.

Maybe we could make this a constant URL in org.apache.webbeans.xml.DefaultBeanArchiveService?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm not sure I got it right but my point is that this code shouldn't be since the url should be injected in the scanner in SE mode so in owb-impl the url should already be seen (thanks addDeploymentUrl call)

what we likely do not want is impl to depend on se (like in this pr)

hope it makes more sense phrased this way

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand :-).

Happy to debug around addDeploymentUrl, and see what I can find. What I can tell you is that if you revert out the BeansDeployer change in this PR, the tests in this PR will fail (or at least they do for me). I'm very happy to look at a different fix (I might have questions.... :-) ), but I do think a fix is needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we all agree the API should be respected and the regression be fixed if any (maybe java related, didnt check if latest jre changed url handling), but think we should stick to the original design of se on top of impl and not a cycle dep and url 100% taken from the scanner, even virtual ones which were just a hack to make it work without a new spi. This part can change if it help - but think we can make it work, there should be something fishy making the fake url dropped somewhere in the process.
can try to help to debug next week if there is no new finding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addDeploymentUrl is called from org.apache.openwebbeans.se.CDISeScannerService#addClassesDeploymentUrl with the embedded URL (great), however, the scanner.getBeanXmls() call from org.apache.webbeans.config.BeansDeployer#deployFromXML does not return that URL, as addClassesDeploymentUrl adds the URL to org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanDeploymentUrls and scanner.getBeanXmls() gets the URLs from org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanArchiveLocations.

Could we just include the embedded URL in org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanArchiveLocations?

Copy link
Contributor Author

@jgallimore jgallimore Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this:

    protected void addDeploymentUrl(String beansXml, URL cpUrl)
    {
        beanDeploymentUrls.put(beansXml, cpUrl);
        beanArchiveLocations.add(cpUrl);
    }

in org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#addDeploymentUrl

works. Would that change be ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess it will need to call doAddWebBeansXmlLocation but looks like the additional call to do

that said doing it in addDeploymentUrl will change the lifecycle for the nominal case so i'd just align on the nominal case so adjust CDISeScannerService (a bit like web scanner has web-inb url handling specifically)

hope it makes sense

@jgallimore
Copy link
Contributor Author

@rmannibucau I appreciate the feedback. I've pushed a different change, which seems to pass all the tests. Hopefully I've understood the feedback and this is a better solution.

@rmannibucau
Copy link
Contributor

except the wildcard import LGTM, thanks

@jgallimore
Copy link
Contributor Author

thanks @rmannibucau !

@jgallimore
Copy link
Contributor Author

Hi folks - would it be possible to get this merged? Also are there any plans for a release that could include this?

@tandraschko tandraschko merged commit 916a772 into apache:main Mar 6, 2025
1 check passed
@tandraschko
Copy link
Member

merged it

@rmannibucau can you do a release? i dont have a setup currently

@jgallimore
Copy link
Contributor Author

That is amazing, thank you so much!

@rmannibucau
Copy link
Contributor

@tandraschko do not have it handy either (full .NET not asf computer at the moment) but can get it back end of the month if needed.

@jgallimore
Copy link
Contributor Author

I'm not a committer, but if I'm able to help in any way, I am obviously very happy to do so. Equally, the request is whether there's a rough time when there might be a release, as opposed to "please can you do a release ASAP". Something at the end of the month would be very awesome and much appreciated. Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants